Skip to content

Remove CreateSignedTransaction deadcode#2479

Open
l0r1s wants to merge 7 commits into
devnet-readyfrom
remove-create-signed-tx-deadcode
Open

Remove CreateSignedTransaction deadcode#2479
l0r1s wants to merge 7 commits into
devnet-readyfrom
remove-create-signed-tx-deadcode

Conversation

@l0r1s
Copy link
Copy Markdown
Collaborator

@l0r1s l0r1s commented Mar 2, 2026

Summary

  • Remove dead CreateSignedTransaction impl and trait bounds — drand only uses send_unsigned_transaction (unsigned extrinsics with a signed payload), so create_signed_transaction was never called
  • Replace deprecated CreateInherent/new_inherent with CreateBare/new_bare across all mock files and runtime

Details

pallet_drand's offchain worker submits pulses via signer.send_unsigned_transaction(), which creates bare extrinsics through CreateBare::create_bare(). The CreateSignedTransaction trait and its implementations (runtime + 6 test mocks) were satisfying a trait bound but never invoked at runtime. This PR removes that dead code (~160 lines) and relaxes pallet_drand::Config to only require CreateBare<Call<Self>> + SigningTypes.

@l0r1s l0r1s added the skip-cargo-audit This PR fails cargo audit but needs to be merged anyway label Mar 2, 2026
sam0x17
sam0x17 previously approved these changes Mar 2, 2026
@l0r1s l0r1s force-pushed the remove-create-signed-tx-deadcode branch from d1395a7 to b9c2830 Compare May 25, 2026 22:38
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 25, 2026

🛡️ AI Review — Skeptic (security review)

VERDICT: SAFE

BASELINE scrutiny: l0r1s has repo write permission, an established 2018 account, substantial contribution history, no Gittensor allowlist hit found, and branch remove-create-signed-tx-deadcode targets devnet-ready.

Static-only review of the prefetched PR context found a narrowly scoped runtime/mock conversion from deprecated CreateInherent/new_inherent plus unused CreateSignedTransaction plumbing to CreateBare/new_bare, with pallet_drand::Config relaxed accordingly. The live drand offchain path still uses send_unsigned_transaction, and unsigned calls still require signed payload verification in validate_unsigned before acceptance. The added github-actions[bot] commit is limited to auto-updated benchmark weights, and the patch contains no .github/ai-review/*, .github/copilot-instructions.md, dependency, lockfile, or build-script changes.

Findings

No findings.

Conclusion

No malicious behavior or security vulnerability was found in this diff. The PR removes dead signed-transaction scaffolding without weakening drand unsigned transaction validation.


🔍 AI Review — Auditor (domain review)

VERDICT: 👍

Gittensor association UNKNOWN by allowlists; author has write permission and substantial recent subtensor history, so reviewed as an established repo contributor without incentive adjustment.

The PR body is substantive and matches the core drand change: the offchain worker still uses send_unsigned_transaction, and the runtime/mock implementations now provide the bare extrinsic path through CreateBare/new_bare while removing unused signed-transaction plumbing.

The diff also carries a mechanical benchmark weight refresh and bumps runtime/src/lib.rs spec_version from 411 to 412. The duplicate-work check found only broad runtime/mock/weight-file overlaps with unrelated open PRs; none appear to implement this dead-code removal.

I attempted targeted compile confirmation with cargo test -p pallet-drand --no-run, but rustup failed before compilation because /home/runner/.rustup/tmp is read-only in this sandbox. git diff --check origin/devnet-ready...HEAD passed. No auto-fixes were applied.

Findings

No findings.

Prior-comment reconciliation

  • a0a987ed: addressed — The current diff adds CreateBare implementations across the affected drand runtime/mock surfaces and removes the stale CreateSignedTransaction/CreateInherent usage.

Conclusion

The prior compile-surface concern is addressed in the current diff, and the remaining changes are narrow or mechanical. I found no blocking domain issues.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review — see the sticky summary comment for the verdict and the inline comments below for specific findings.

Comment thread pallets/drand/src/lib.rs
pub trait Config:
CreateSignedTransaction<Call<Self>> + CreateInherent<Call<Self>> + frame_system::Config
{
pub trait Config: CreateBare<Call<Self>> + SigningTypes + frame_system::Config {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Update all drand mock runtimes to implement CreateBare

pallet_drand::Config now requires CreateBare<Call<Self>>, but this PR only converts part of the repo. Existing pallet_drand::Config implementors in pallets/subtensor/src/tests/mock_high_ed.rs, precompiles/src/mock.rs, and eco-tests/src/mock.rs still implement CreateInherent/CreateSignedTransaction and do not implement CreateBare, so crates that compile those mocks will no longer satisfy this bound. Convert those remaining mocks the same way as the files in this PR: implement frame_system::offchain::CreateBare, use UncheckedExtrinsic::new_bare(...), and remove the now-unused CreateSignedTransaction implementations.

@github-actions
Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👎

@l0r1s l0r1s force-pushed the remove-create-signed-tx-deadcode branch from b9c2830 to 0febfba Compare May 26, 2026 14:56
@github-actions
Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@github-actions
Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

@l0r1s
Copy link
Copy Markdown
Collaborator Author

l0r1s commented May 26, 2026

Hello @basfroman and @ibraheem-abe it seems that the job to find btcli tests fails but I'm not really sure why, could you please have a look? Thanks!

open-junius
open-junius previously approved these changes May 27, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🔄 AI review updated — Skeptic: SAFE Auditor: 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-cargo-audit This PR fails cargo audit but needs to be merged anyway

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants